test(mapper): strengthen concurrency and nested-AND coverage - #187
Merged
Conversation
Make ReflectionDatabindTest.fireOnlyOnce launch 32 threads gated on a start barrier so they genuinely contend on the AtomicBoolean CAS + CountDownLatch dedup in getFieldValue(), while still asserting the inner reflective read runs exactly once per Databind. Fix the NestedInNestedAnd.name annotation from @NestedSpec to @SPEC (mirroring NestedInNestedOr) so the depth-3 AND scenario actually composes a predicate, and extend allAnd's assertions to a full three-level Conjunction tree symmetric to allOr's Disjunction tree. Refs #182 Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
shihyuho
commented
Jul 16, 2026
shihyuho
left a comment
shihyuho
left a comment
Member
Author
There was a problem hiding this comment.
APPROVE (advisory) — reviewed all 9 axes, no must-fix
Non-blocking (4)
- Optional · Spec conformance —
mapper/src/test/java/tw/com/softleader/data/jpa/spec/ReflectionDatabindTest.java:56—Closes #182will auto-close the issue while TEST-06's second clause (a bind whose reflective read throws) is neither implemented nor declared out of scope. - Nit · Correctness —
mapper/src/test/java/tw/com/softleader/data/jpa/spec/ReflectionDatabindTest.java:67— the worker-threadthrow new RuntimeException(e)lands in a discardedFuture, sofireOnlyOncestays green even if every loser thread blows up. - Nit · Correctness —
mapper/src/test/java/tw/com/softleader/data/jpa/spec/ReflectionDatabindTest.java:73—startBarrier.await()anddone.await()are unbounded, so a regression in the very latch machinery this test guards would hang the build instead of failing it. - Nit · Test coverage —
mapper/src/test/java/tw/com/softleader/data/jpa/spec/NestedSpecificationResolverTest.java:395— flippingNestedInNestedAnd.nameto@Specremoves the suite's only coverage ofNestedSpecificationResolver's null-collapse branch.
Coverage & checks
| Axis | Status |
|---|---|
| Correctness | 2 findings |
| Spec conformance | 1 finding |
| Scope | clean — every hunk traced to TEST-06/TEST-05 of the linked issue; only the two test files touched |
| Convention | clean — checked the base-ref CLAUDE.md / AGENTS.md / CONTRIBUTING.md; AGENTS.md's only rule (commit hygiene) is CI-enforced and lands on no changed line |
| Security | clean — test-only; no secrets or string-built queries introduced, specs still constructed through the typed Criteria API |
| Readability | clean — the latch→done rename disambiguates the two synchronizers; depth2 matches the allOr/mix idiom already in the file |
| Architecture | clean — NestedInNestedAnd now mirrors NestedInNestedOr; the assertion navigation is byte-identical to the established pattern |
| Performance | clean — production sources re-fetched at the head SHA and confirmed unchanged; 32 threads costs single-digit ms |
| Test coverage | 1 finding |
- Verified — all 4 checks green:
commits,pr-title,continuous-integration/jenkins/branch,continuous-integration/jenkins/pr-merge(read viagh pr checks, never run locally) - Out-of-band — re-fetched
ReflectionDatabind,NestedSpecificationResolverandSpecMapperat the head SHA to confirm no production line changed · tracedThreadPoolExecutorcore-thread behaviour to rule out aCyclicBarrier(32)deadlock · confirmed Mockito's invocation recording is synchronized, so the 32-threadverifyis race-free
🤖 Reviewed by Claude Opus 4.8
(self-review — same account; GitHub records this as event=COMMENT with no badge, so the verdict above is advisory.)
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Strengthens two mapper tests so they actually exercise the code paths they claim to cover (WP10, findings TEST-06 and TEST-05). Test-only change — no production code touched.
ReflectionDatabindTest.fireOnlyOncepreviously ran a single-threaded pool (numberOfThreads = 1), so theAtomicBooleanCAS +CountDownLatchdedup inReflectionDatabind.getFieldValue()was never contended and thetimes(1)assertion would pass even if the dedup were deleted. It now launches 32 threads held at aCyclicBarrierand released together, so multiple threads race intogetFieldValue()on the sameDatabindinstances, while still asserting the inner reflective read runs exactly once per bind.NestedInNestedAnd.namewas annotated@NestedSpec String name(a typo vs its OR twin's@Spec), so recursing into aStringyieldednulland the deepest AND level was silently absent. Changed to@Spec(mirroringNestedInNestedOr) and extendedallAnd's assertions to a full three-levelConjunctiontree symmetric toallOr'sDisjunctiontree.TEST-03 (inherited criteria fields) is deliberately out of scope — it needs a human policy decision and is tracked separately.
Closes #182
Acceptance criteria
ReflectionDatabindTest.fireOnlyOncelaunches 32 concurrent threads held at a start barrier and released together, so multiple threads call the sameReflectionDatabind.getFieldValue()concurrently.Databindreturned byReflectionDatabind.of(...), the test asserts the innergetFieldValue(object, field)read is invoked exactly once (times(1)) despite the concurrent access.NestedInNestedAnd.nameis annotated@Spec(matchingNestedInNestedOr.name).allAndasserts a three-levelConjunctiontree: top-levelConjunctionof size 2 (Equals+ nestedConjunction), nestedConjunctionof size 2 (Equals+ deepestConjunction), deepestConjunctionof size 1 containing anEquals.allAnd's repository result still returns exactlymatt.mvn test(mapper module) passes.Testing
mvn testrun on JDK 17 — full reactor green:specification-mapperandspecification-mapper-starterboth SUCCESS. The two affected classes:ReflectionDatabindTest(1 test) andNestedSpecificationResolverTest(5 tests) pass.